Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check for null when getting DTV values (JDBC spec compliance - getBinaryStream /getAsciiStream will return null when the value is null) #2600

Merged
merged 4 commits into from
Feb 20, 2025
Merged

Check for null when getting DTV values (JDBC spec compliance - getBinaryStream /getAsciiStream will return null when the value is null) #2600

merged 4 commits into from
Feb 20, 2025

Conversation

machavan
Copy link
Contributor

@machavan machavan commented Feb 11, 2025 •
edited by lilgreenbird
Loading

This fix of checking for null value of a DTVImpl before calling DTVImpl::getValue has shown some perf gains in our internal benchmark.

This fix will change behavior of getBinaryStream /getAsciiStream in ResultSet to be compliant with JDBC specs:
a Java input stream that delivers the database column value as a stream of one-byte ASCII characters; if the value is SQL NULL, the value returned is null

@machavan machavan changed the title Perf fix 1 - Add a null value check upfront in dtv::getValue Perf fix 1 - Add a null value check upfront in DTV::getValue Feb 11, 2025
@codecov Codecov
Copy link

codecov bot commented Feb 11, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.33%. Comparing base (e63814e) to head (01e79e9).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #2600      +/-   ##
============================================
- Coverage     51.35%   51.33%   -0.03%     
+ Complexity     3981     3970      -11     
============================================
  Files           147      147              
  Lines         33686    33688       +2     
  Branches       5627     5628       +1     
============================================
- Hits          17300    17294       -6     
+ Misses        13956    13953       -3     
- Partials       2430     2441      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jeffery-Wasty Jeffery-Wasty added this to the 12.10.0 milestone Feb 11, 2025
@Jeffery-Wasty Jeffery-Wasty linked an issue Feb 11, 2025 that may be closed by this pull request
Slowness in MS SQL Server JDBC driver #2432
Open
@Jeffery-Wasty Jeffery-Wasty removed a link to an issue Feb 11, 2025
Slowness in MS SQL Server JDBC driver #2432
Open
lilgreenbird
lilgreenbird previously requested changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@lilgreenbird lilgreenbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add test?

@muskan124947
Copy link
Contributor

Ado pipeline : https://sqlclientdrivers.visualstudio.com/mssql-jdbc/_build/results?buildId=108714&view=results

@machavan
Copy link
Contributor Author

add test?

Verified that existing tests already cover this code path. This code path is hit for any get call on a ResultSet that returns NULL. e.g. many tests in ResultSetTest cover this.

@muskan124947 muskan124947 merged commit b0ff5b1 into main Feb 20, 2025
18 of 19 checks passed
@muskan124947 muskan124947 deleted the dev/machavan/perffix1 branch February 20, 2025 08:38
@lilgreenbird lilgreenbird changed the title Perf fix 1 - Add a null value check upfront in DTV::getValue Check for null when getting DTV values (java compliance - getAsciiStream will return null when the value is null) Feb 27, 2025
@lilgreenbird lilgreenbird changed the title Check for null when getting DTV values (java compliance - getAsciiStream will return null when the value is null) Check for null when getting DTV values (JDBC spec compliance - getAsciiStream will return null when the value is null) Feb 27, 2025
@lilgreenbird lilgreenbird changed the title Check for null when getting DTV values (JDBC spec compliance - getAsciiStream will return null when the value is null) Check for null when getting DTV values (JDBC spec compliance - getBinaryStream /getAsciiStream will return null when the value is null) Feb 27, 2025
@lilgreenbird lilgreenbird mentioned this pull request Feb 28, 2025
Merged
@lilgreenbird
Copy link
Contributor

test added in PR #2617

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divang divang divang approved these changes

@Jeffery-Wasty Jeffery-Wasty Jeffery-Wasty approved these changes

@David-Engel David-Engel David-Engel approved these changes

@tkyc tkyc tkyc approved these changes

@lilgreenbird lilgreenbird lilgreenbird left review comments

Assignees

@machavan machavan

Labels
None yet
Projects
MSSQL JDBC
Status: Closed/Merged PRs
Milestone
12.10.0
Development

Successfully merging this pull request may close these issues.
7 participants
@machavan @muskan124947 @lilgreenbird @divang @Jeffery-Wasty @David-Engel @tkyc